make a webhook to remind us to run the needful make commands#4659
make a webhook to remind us to run the needful make commands#4659
Conversation
|
Skipping CI for Draft Pull Request. |
rajdeepc2792
left a comment
rajdeepc2792
left a comment
There was a problem hiding this comment.
Two Arguments:
- Can we do a pre-push rather than a pre-commit?
- I am in with keeping it as simple as asking question, but can we advance it to notify only when no change is detected after running the required make?
|
| echo " - make generate" | ||
| echo " - make imports" | ||
| read -r -p "Have you run all required make commands? [y/N] " run_make_commands | ||
| if [[ ! "${run_make_commands}" =~ ^([yY][eE][sS]|[yY])$ ]]; then |
There was a problem hiding this comment.
Elaborating on the second point:
- Executing all the required make targets as part of the script
- Running
git status --porcelainto detect modified or untracked files - Fail and ask to run the required make commands and keep the local repo clean after commits.
PS: I always have some modified/untracked files on my local, so I would not prefer this flow. But a suggestion.
I believe asking every time during push is fine.
There was a problem hiding this comment.
I didn't want to have to maintain all the required make commands within the script hence this implementation just being a nag vs total automation.
| echo " - make generate" | ||
| echo " - make imports" | ||
| read -r -p "Have you run all required make commands? [y/N] " run_make_commands | ||
| if [[ ! "${run_make_commands}" =~ ^([yY][eE][sS]|[yY])$ ]]; then |
There was a problem hiding this comment.
I didn't want to have to maintain all the required make commands within the script hence this implementation just being a nag vs total automation.
hlipsig
requested review from
bennerv,
cadenmarchese,
hawkowl,
jharrington22,
mrWinston,
rogbas,
tiguelu and
yjst2012
as code owners
hlipsig
requested review from
alcasim,
kimorris27,
mociarain,
sankur-codes,
tsatam,
tuxerrante,
ventifus and
wanghaoran1988
as code owners
There was a problem hiding this comment.
Pull request overview
Adds a local Git hook intended to remind contributors to run commonly-forgotten make targets before pushing, helping prevent CI failures due to missing generated/import updates.
Changes:
- Introduce a new
pre-pushhook that prompts developers to confirm they ran requiredmakecommands. - Simplify the existing
pre-commithook by removing an explicitexit 0at the end.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
hack/git/hooks/pre-push |
New interactive pre-push check prompting users to run key make targets before pushing. |
hack/git/hooks/pre-commit |
Minor cleanup: remove redundant trailing exit 0. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| read -r -p "Have you run all required make commands? [y/N] " run_make_commands | ||
| if [[ ! "${run_make_commands}" =~ ^([yY][eE][sS]|[yY])$ ]]; then | ||
| echo "Commit aborted. Run the required make commands and try again." | ||
| exit 1 |
There was a problem hiding this comment.
pre-push hooks receive the list of refs to push on stdin. Using read -p without redirecting will read from that stdin (consuming a ref line) instead of from the user, which can cause the hook to abort pushes unexpectedly (often every time). Read the confirmation from /dev/tty (or similar) and/or guard for non-interactive environments so pushes from automation/IDEs don't hang or fail due to EOF.
| read -r -p "Have you run all required make commands? [y/N] " run_make_commands | |
| if [[ ! "${run_make_commands}" =~ ^([yY][eE][sS]|[yY])$ ]]; then | |
| echo "Commit aborted. Run the required make commands and try again." | |
| exit 1 | |
| # Only prompt when an interactive terminal is available. | |
| if [ -t 0 ] || [ -t 1 ]; then | |
| # Read from /dev/tty so we don't consume Git's stdin (the list of refs). | |
| if ! read -r -p "Have you run all required make commands? [y/N] " run_make_commands </dev/tty; then | |
| # If we cannot read from the terminal, allow the push to proceed. | |
| exit 0 | |
| fi | |
| if [[ ! "${run_make_commands}" =~ ^([yY][eE][sS]|[yY])$ ]]; then | |
| echo "Commit aborted. Run the required make commands and try again." | |
| exit 1 | |
| fi |
There was a problem hiding this comment.
Copilot's comment here makes sense to me, but we're probably fine without it. I can't imagine when/why we'd want automation pushing to this repo.
| echo " - make imports" | ||
| read -r -p "Have you run all required make commands? [y/N] " run_make_commands | ||
| if [[ ! "${run_make_commands}" =~ ^([yY][eE][sS]|[yY])$ ]]; then | ||
| echo "Commit aborted. Run the required make commands and try again." |
There was a problem hiding this comment.
This message says "Commit aborted" but this is a pre-push hook; it should say "Push aborted" (or similar) to avoid confusion.
| echo "Commit aborted. Run the required make commands and try again." | |
| echo "Push aborted. Run the required make commands and try again." |
kimorris27
left a comment
kimorris27
left a comment
There was a problem hiding this comment.
I think we should go with one of Copilot's suggestions, but LGTM otherwise!
| read -r -p "Have you run all required make commands? [y/N] " run_make_commands | ||
| if [[ ! "${run_make_commands}" =~ ^([yY][eE][sS]|[yY])$ ]]; then | ||
| echo "Commit aborted. Run the required make commands and try again." | ||
| exit 1 |
There was a problem hiding this comment.
Copilot's comment here makes sense to me, but we're probably fine without it. I can't imagine when/why we'd want automation pushing to this repo.
| exit 1 | ||
| fi | ||
|
|
||
| exit 0 |
There was a problem hiding this comment.
Nit: Did you mean to put this here? Asking since you removed it from the existing pre-commit hook.
What this PR does / why we need it:
Should help developers close the loop between a code change and a forgotten make command captured by CI.